Don't execute the proxy queue while adding to it from same thread. #24565

brendandahl · 2025-06-13T21:32:01Z

test_pthread_cancel was intermittently failing with a deadlock. From the backtrace, I found we were adding to the queue which can sometimes call malloc->emscripten_yield->emscripten_proxy_execute_queue which also tries to lock the same queue on the same thread.

brendandahl · 2025-06-13T21:32:23Z

Still working on a more reliable test, but I wanted to try this out...

system/lib/pthread/proxying.c

sbc100

Nice work!

Hopefully we can also figure out some way to improve the testing ?

system/lib/pthread/proxying.c

test/code_size/random_printf_wasm2js.json

system/lib/pthread/proxying.c

test/code_size/random_printf_wasm2js.json

test/other/codesize/test_codesize_minimal_pthreads.funcs

tlively · 2025-06-16T21:40:01Z

Solution LGTM, but I forgot about the testing.

brendandahl · 2025-06-17T16:36:11Z

Changing to using a thread local causes failures in test_pthread_run_on_main_thread_minimal_runtime. I didn't get very far debugging this. I won't be able to work on this until June ~25th, so if anyone wants to pick it up feel free to.

Also use `bool` rather than `int` to better convey intent. Split out from emscripten-core#24565

There was some suspicion that this might lead to a code size change but it doesn't seem to (I suppose because bool/int doesn't have any code in its constructor). Also use `bool` rather than `int` to better convey intent. Split out from #24565

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

TLS was not initialized on workers causing the tls_base address to be 0 and write to the wrong memory location. Fixes emscripten-core#21528 and is also needed for PR emscripten-core#24565

TLS was not initialized on workers causing the tls_base address to be 0 and write to the wrong memory location. Fixes #21528 and is also needed for PR #24565

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

test_pthread_cancel was intermittently failing with a deadlock. From the backtrace, I found we were adding to the queue which can sometimes call malloc->emscripten_yield->emscripten_proxy_execute_queue which also tries to lock the same queue on the same thread.

brendandahl · 2025-07-07T23:51:15Z

Unfortunately, I still haven't found a reliable way to reproduce this in a test case. Are we fine to land without?

system/lib/pthread/proxying.c

tlively · 2025-07-08T00:41:36Z

Does this test sketch sound plausible?

Main thread spawns thread 1 and spins
Thread 1 asynchronously sends a set-flag message to the main thread

This allocates and fills a task_queues buffer of size 1.

Thread 1 signals main thread to continue
Main thread stops spinning and asynchronously sends a nop task to thread 1.

This allocates and fills a task_queues buffer of size 2.

Main thread does not deadlock on the previous step
Main thread asserts that the flag is not set, showing that the task queue has not been executed.

Maybe we can also hook the allocator to set flags as well and prove that the allocations happened at the expected times during the test. We do something similar when testing frees here: https://github.com/emscripten-core/emscripten/blob/main/test/pthread/test_pthread_proxying_refcount.c#L25-L28.

If that doesn't sound plausible, then testing by observing fewer flakes SGTM.

sbc100 · 2025-07-08T00:55:10Z

@tlively (I added some numbers to your steps above). Is the expectation that we could make step 4 deterministically fail without this PR?

tlively · 2025-07-08T01:03:30Z

Yes, that's what I was aiming for. I haven't tried it, though :)

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

This would really have helped in at least detecting the deadlock that was found in #24570 / #24565

test/pthread/test_pthread_proxy_deadlock.c

sbc100

lgtm % test comments

sbc100 · 2025-07-09T21:58:21Z

test/pthread/test_pthread_proxy_deadlock.c

+
+int main() {
+  pthread_create(&looper, NULL, looper_main, NULL);
+  emscripten_proxy_async(emscripten_proxy_get_system_queue(), looper, run_on_looper, NULL);


Could this ever set should_quit before looper_main actually starts running?

Can you add some comments somewhere in this file about exactly that case we are testing for.

I kind of assumed the thread start would be guaranteed to run before any proxy messages, but now I'm not sure. Even if it does run before, we'd still hit a deadlock since the main thread will be locked during task creation and trying to run the queue and lock again. It doesn't really matter what the other thread is doing.

You can have looper_main set a flag and have main wait for that flag to be set before doing the proxying to be sure. +1 to adding comments, especially about how the deadlock would occur without the bug fix.

I changed the test a bit more and removed the looper thread and just sends the task to the same thread. This makes the test a little less realistic, but highlights it doesn't really matter what the thread is doing.

sbc100 · 2025-07-09T23:31:28Z

test/pthread/test_pthread_proxy_deadlock.c

+  // 3. Malloc then attempts to execute and lock the already-locked queue,
+  //    causing a deadlock.
+  // This test ensures our implementation prevents this re-entrant lock.
+  emscripten_proxy_async(emscripten_proxy_get_system_queue(), pthread_self(), task, NULL);


Oh nice, so you don't even need a second thread! Makes sense I guess since this is a double-lock/self-deadlock issue.

Is it valid to send work to yourself?

test_pthread_proxying.c has a test for proxing to itself, so I assume so.

Yep, but only asynchronous self-proxying is allowed, since synchronous self-proxying would obviously deadlock.

brendandahl marked this pull request as draft June 13, 2025 21:32

sbc100 reviewed Jun 13, 2025

View reviewed changes

system/lib/pthread/proxying.c Outdated Show resolved Hide resolved

kleisauke mentioned this pull request Jun 14, 2025

Pthread task queue deadlock race condition problem (with pthread_cancel()?) #24570

Open

brendandahl force-pushed the proxy-deadlock branch from 9bf0082 to fcffdb1 Compare June 16, 2025 21:08

sbc100 reviewed Jun 16, 2025

View reviewed changes

system/lib/pthread/proxying.c Outdated Show resolved Hide resolved

test/code_size/random_printf_wasm2js.json Outdated Show resolved Hide resolved

brendandahl force-pushed the proxy-deadlock branch from fcffdb1 to ff117cf Compare June 16, 2025 21:36

tlively approved these changes Jun 16, 2025

View reviewed changes

system/lib/pthread/proxying.c Outdated Show resolved Hide resolved

system/lib/pthread/proxying.c Outdated Show resolved Hide resolved

test/code_size/random_printf_wasm2js.json Outdated Show resolved Hide resolved

tlively reviewed Jun 16, 2025

View reviewed changes

test/other/codesize/test_codesize_minimal_pthreads.funcs Outdated Show resolved Hide resolved

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 17, 2025

Use global TLS variable in proxying.c. NFC

9ee6223

Also use `bool` rather than `int` to better convey intent. Split out from emscripten-core#24565

sbc100 mentioned this pull request Jun 17, 2025

Use global TLS variable in proxying.c. NFC #24596

Merged

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 17, 2025

Use global TLS variable in proxying.c. NFC

7b3f8fc

Also use `bool` rather than `int` to better convey intent. Split out from emscripten-core#24565

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 17, 2025

Use global TLS variable in proxying.c. NFC

d27dc0c

Also use `bool` rather than `int` to better convey intent. Split out from emscripten-core#24565

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 18, 2025

Add pthread mutex deadlock detection for debug builds

a57b588

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

sbc100 mentioned this pull request Jun 18, 2025

Add pthread mutex deadlock detection for debug builds #24607

Merged

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 18, 2025

Add pthread mutex deadlock detection for debug builds

0e10a5e

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 19, 2025

Add pthread mutex deadlock detection for debug builds

efb88c5

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 19, 2025

Add pthread mutex deadlock detection for debug builds

a8d3d3f

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

brendandahl mentioned this pull request Jul 2, 2025

[pthreads] Initialize TLS on workers in MINIMAL_RUNTIME mode. #24646

Merged

brendandahl force-pushed the proxy-deadlock branch from ff117cf to 9ca233d Compare July 2, 2025 23:04

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 7, 2025

Add pthread mutex deadlock detection for debug builds

e59d085

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

brendandahl marked this pull request as ready for review July 7, 2025 23:48

brendandahl added 3 commits July 7, 2025 23:48

use previous gaurd

91fa30f

comment fix

a76f7e2

brendandahl force-pushed the proxy-deadlock branch from 9ca233d to a76f7e2 Compare July 7, 2025 23:49

brendandahl requested a review from sbc100 July 7, 2025 23:50

sbc100 reviewed Jul 7, 2025

View reviewed changes

system/lib/pthread/proxying.c Outdated Show resolved Hide resolved

sbc100 approved these changes Jul 7, 2025

View reviewed changes

system/lib/pthread/proxying.c Outdated Show resolved Hide resolved

system/lib/pthread/proxying.c Outdated Show resolved Hide resolved

brendandahl added 2 commits July 7, 2025 23:54

bool

973f709

mo bool

9528e48

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 8, 2025

Add pthread mutex deadlock detection for debug builds

c06ae77

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 8, 2025

Add pthread mutex deadlock detection for debug builds

cf6ce4a

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 8, 2025

Add pthread mutex deadlock detection for debug builds

a35942f

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

brendandahl added 2 commits July 8, 2025 22:13

add test

7c24ee8

skip asan/lsan

2b6857c

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 8, 2025

Add pthread mutex deadlock detection for debug builds

2b24c68

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 8, 2025

Add pthread mutex deadlock detection for debug builds

264052a

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 8, 2025

Add pthread mutex deadlock detection for debug builds

42fb00c

This would really have helped in at least detecting the deadlock that was found in emscripten-core#24570 / emscripten-core#24565

sbc100 added a commit that referenced this pull request Jul 9, 2025

Add pthread mutex deadlock detection for debug builds (#24607)

4780cc2

This would really have helped in at least detecting the deadlock that was found in #24570 / #24565

sbc100 reviewed Jul 9, 2025

View reviewed changes

test/pthread/test_pthread_proxy_deadlock.c Outdated Show resolved Hide resolved

remove check for static data

1b8ce30

brendandahl requested a review from tlively July 9, 2025 21:58

sbc100 approved these changes Jul 9, 2025

View reviewed changes

Add comments. Remove pthread and just send the task to self.

1fd8cb8

sbc100 reviewed Jul 9, 2025

View reviewed changes

sbc100 approved these changes Jul 9, 2025

View reviewed changes

tlively approved these changes Jul 10, 2025

View reviewed changes

Don't execute the proxy queue while adding to it from same thread. #24565

Are you sure you want to change the base?

Don't execute the proxy queue while adding to it from same thread. #24565

Conversation

brendandahl commented Jun 13, 2025

Uh oh!

brendandahl commented Jun 13, 2025

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tlively commented Jun 16, 2025

Uh oh!

brendandahl commented Jun 17, 2025

Uh oh!

brendandahl commented Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tlively commented Jul 8, 2025 • edited by sbc100 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Jul 8, 2025

Uh oh!

tlively commented Jul 8, 2025

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

brendandahl Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

tlively Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

brendandahl Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

brendandahl Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

tlively Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tlively commented Jul 8, 2025 •

edited by sbc100

Loading